Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

btree: add size info for google/btree #1735

Merged
merged 15 commits into from
Sep 18, 2019
Merged

btree: add size info for google/btree #1735

merged 15 commits into from
Sep 18, 2019

Conversation

Luffbee
Copy link
Contributor

@Luffbee Luffbee commented Sep 6, 2019

What problem does this PR solve?

The b-tree does not support "get rank", "get k-th" operations because it does not have size info of subtree.

What is changed and how it works?

Add size info for it.

Check List

Tests

  • Unit test

Side effects

  • Increased code complexity

@Luffbee
Copy link
Contributor Author

Luffbee commented Sep 6, 2019

/rebuild

@nolouch
Copy link
Contributor

nolouch commented Sep 9, 2019

Can we split into two PRs? one for port the origin btree, and another for your code.

@nolouch nolouch added the component/util Utility. label Sep 9, 2019
@Luffbee
Copy link
Contributor Author

Luffbee commented Sep 9, 2019

Can we split into two PRs? one for port the origin btree, and another for your code.

Is it necessary? I only put the btree in pkg in this pr, the dependencies are not changed.

disksing
disksing reviewed Sep 10, 2019
View reviewed changes
pkg/btree/btree.go Outdated Show resolved Hide resolved
disksing
disksing reviewed Sep 10, 2019
View reviewed changes
pkg/btree/btree.go Outdated Show resolved Hide resolved
disksing
disksing reviewed Sep 10, 2019
View reviewed changes
pkg/btree/btree.go Outdated Show resolved Hide resolved
disksing
disksing reviewed Sep 10, 2019
View reviewed changes
pkg/btree/btree.go Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Sep 10, 2019 •
edited
Loading

Codecov Report

Merging #1735 into master will increase coverage by 0.28%.
The diff coverage is 86.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1735      +/-   ##
==========================================
+ Coverage   77.05%   77.33%   +0.28%     
==========================================
  Files         162      163       +1     
  Lines       15910    16408     +498     
==========================================
+ Hits        12259    12689     +430     
- Misses       2627     2667      +40     
- Partials     1024     1052      +28
Impacted Files Coverage Δ
pkg/btree/btree.go 86.14% <86.14%> (ø)
server/kv/etcd_kv.go 70.12% <0%> (-9.1%) ⬇️
server/schedulers/random_merge.go 62.5% <0%> (-5%) ⬇️
server/region_syncer/client.go 78.94% <0%> (-3.95%) ⬇️
server/tso/tso.go 79.81% <0%> (-1.84%) ⬇️
client/client.go 68.6% <0%> (-1.3%) ⬇️
server/grpc_service.go 58.78% <0%> (-0.44%) ⬇️
server/member/leader.go 80.1% <0%> (+3.06%) ⬆️
pkg/etcdutil/etcdutil.go 88.4% <0%> (+11.59%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 061eac6...cc04810. Read the comment docs.

@Luffbee
Copy link
Contributor Author

Luffbee commented Sep 11, 2019

@disksing addressed your comments, PTAL

disksing
disksing reviewed Sep 11, 2019
View reviewed changes
pkg/btree/LICENSE
Derivative Works a copy of this License; and

(b) You must cause any modified files to carry prominent notices
stating that You changed the files; and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to describe what we changed at the head of btree.go.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL @Luffbee

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change description added.

@disksing
Copy link
Contributor

The rest LGTM

nolouch
nolouch reviewed Sep 11, 2019
View reviewed changes
Copy link
Contributor

@nolouch nolouch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rest LGTM

pkg/btree/btree_test.go Outdated Show resolved Hide resolved
pkg/btree/btree_test.go Outdated Show resolved Hide resolved
pkg/btree/btree_test.go Show resolved Hide resolved
nolouch
nolouch approved these changes Sep 11, 2019
View reviewed changes
Copy link
Contributor

@nolouch nolouch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nolouch nolouch added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 11, 2019
shafreeck
shafreeck reviewed Sep 12, 2019
View reviewed changes
pkg/btree/btree.go Outdated Show resolved Hide resolved
@nolouch
Copy link
Contributor

nolouch commented Sep 18, 2019

PTAL @disksing

@disksing disksing merged commit 4ca2265 into tikv:master Sep 18, 2019
@Luffbee Luffbee deleted the btree-size-info branch October 15, 2019 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nolouch nolouch nolouch approved these changes

@shafreeck shafreeck shafreeck left review comments

@disksing disksing disksing approved these changes

@rleungx rleungx Awaiting requested review from rleungx

Assignees
No one assigned
Labels
component/util Utility. status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Luffbee @nolouch @codecov-io @disksing @shafreeck